Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor/add notification after adding a proposal #620

Draft
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

BarbaraOliveira13
Copy link
Contributor

🎩 Description

Please describe your pull request.

📌 Related Issues

Link your PR to an issue

Testing

Describe the best way to test or validate your PR.

Example:

  • Log in as admin
  • Access Backoffice
  • Go to organization settings
  • See ...

Tasks

  • Add specs
  • Add note about overrides in OVERLOADS.md
  • In case of new dependencies or version bump, update related documentation

📷 Screenshots

Please add screenshots of the changes you're proposing if related to the UI

Copy link
Contributor

@Quentinchampenois Quentinchampenois left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, here is the next steps on this PR

We must :

  • fix the event specs
  • add the correct translations keys in english and french

Command to help :

  • generate your test_app using bundle exec rake test:setup
  • Normalize i18n keys using bundle exec i18n-tasks normalize --locales en,fr
  • Lint ruby files using bundle exec rubocop -a
  • Run specs using bundle exec rspec spec/events/decidim/proposals/proposal_published_event_spec.rb

def send_publication_notification
Decidim::EventsManager.publish(
event: "decidim.events.proposals.proposal_published_event",
event_class: Decidim::Proposals::ProposalPublishedEvent,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
event_class: Decidim::Proposals::ProposalPublishedEvent,
event_class: Decidim::Proposals::AuthorConfirmationProposalEvent,

# app/events/decidim/proposals/proposal_published_event.rb
module Decidim
module Proposals
class ProposalPublishedEvent < Decidim::Events::SimpleEvent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
class ProposalPublishedEvent < Decidim::Events::SimpleEvent
class AuthorConfirmationProposalEvent < Decidim::Events::SimpleEvent

To be easier to read, we should rename the class Event as follow and also rename the file to app/events/decidim/proposals/author_confirmation_proposal_event.rb

@@ -67,6 +67,12 @@ fr:
new:
sign_in_disabled: Vous pouvez accéder avec un compte externe
events:
proposals:
proposal_published_event:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
proposal_published_event:
author_confirmation_proposal_event:

@@ -158,6 +164,11 @@ fr:
collaborative_drafts_list: Accéder aux brouillons collaboratifs
new_proposal: Nouvelle proposition
view_proposal: Voir la proposition
notifications:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This locale key is no longer needed because it is called above with notification_title key

describe "email_subject" do
context "when resource title contains apostrophes" do
it "is generated correctly" do
expect(subject.email_subject).to eq("New proposal \"#{resource_title}\" by @#{author.nickname}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should match the expected translation

end

it "is generated correctly" do
expect(subject.email_subject).to eq("New proposal \"#{resource_title}\" by @#{author.nickname}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should match the expected translation

describe "email_intro" do
it "is generated correctly" do
expect(subject.email_intro)
.to eq("#{author.name} @#{author.nickname}, who you are following, has published a new proposal called \"#{resource_title}\". Check it out and contribute:")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should match the expected translation

describe "email_outro" do
it "is generated correctly" do
expect(subject.email_outro)
.to eq("You have received this notification because you are following @#{author.nickname}. You can stop receiving notifications following the previous link.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should match the expected translation

describe "notification_title" do
it "is generated correctly" do
expect(subject.notification_title)
.to include("The <a href=\"#{resource_path}\">#{resource_title}</a> proposal was published by ")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should match the expected translation

@@ -147,6 +147,11 @@ en:
exports:
awesome_private_proposals: Proposals with private fields
proposal_comments: Comments
notifications:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

English keys must match french keys

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants